🧹 chore: code quality audit — fix critical, high, medium, and low bugs#15
Merged
🧹 chore: code quality audit — fix critical, high, medium, and low bugs#15
Conversation
- F04: add missing comma in Purge-InstalledModules module name array
- F06: add missing -Value to Set-Variable in Get-ADUserTransitiveGroupMembership
- F08: add missing -Value to Set-Variable in Export-AllADUserTransitiveGroupMemberships
- F10: fix broken AD -Filter (single-quoted string) in Export-AllGroupMemberships
- F12: fix ValidateSet mismatch ('Office' -> 'physicalDeliveryOfficeName') in Get-ADAttributeUniqueValues
- F13/F15: convert empty hard-coded \ to mandatory parameter; fix date format (hh-m-ss -> HH-mm-ss); add TODO comments on unreplaceable placeholders in Archive-ObsoleteGroups
- F21: fix Select-Object -ExpandProperty with multiple properties in Get-FSMORoleDetails (only accepts one); cache Forest/Domain objects and access properties directly
- F23: fix \$._Index typo -> \ in Update-DnsServerList
- F26: replace Enter-PSSession/Exit-PSSession (interactive-only) with Invoke-Command -Session in Push-DNSClientServerAddresses; add Remove-PSSession in finally block
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… general scripts
F07: Move Sort-Object emit from end to process block in Get-ADUserTransitiveGroupMembership
to prevent pipeline clobbering (only last input was returned to end block)
F09: Fix undefined \\\ -> \\.Count\ in Export-AllADUserTransitiveGroupMemberships
F16: Fix operator precedence bug in Get-InactiveADUser (add parens to -not comparison)
F17: Use \\\ parameter value as export path when provided in Get-InactiveADUser
F19: Replace \continue\ with \
eturn\ in catch outside loop in Get-LockedOutLocation
F22: Move pipeline identity resolution from begin to process block in Get-ADObjectFromPipeline
F24: Make \\\ mandatory in Update-ModuleVersion to prevent null member access
F27: Replace empty placeholder strings with mandatory params in Push-DNSClientServerAddresses
F29: Remove hardcoded DOMAINNAME.org placeholder from Exchange ConnectionUri
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
F02: Fix RegistryAccessRule from FullControl to ReadKey in Activate and Get License
(comment/output said 'read permissions' but code was granting FullControl)
F05: Fix Write-Error \\Cannot validate argument on parameter 'MaximumHistoryCount'. The 0 argument is less than the minimum allowed range of 1. Supply an argument that is greater than or equal to 1 and then try the command again.[0]\ -> \\\ in Get Hostnames from CSV IP Addresses
(\\Cannot validate argument on parameter 'MaximumHistoryCount'. The 0 argument is less than the minimum allowed range of 1. Supply an argument that is greater than or equal to 1 and then try the command again.[0]\ may not reflect the current exception in a catch block)
F15: Confirmed already fixed in critical pass (Get-Date format HH-mm-ss correct)
F18: Replace \�reak 1\ with \
eturn\ in two catch blocks in Get-InactiveUsers
(break outside a loop throws LoopFlowException in PowerShell)
F20: Add BadPasswordTime to Get-ADUser -Properties list in Get-LockedOutLocation
(property was used in output object but never requested from AD)
F25: Fix undefined \\\ -> \\\\ in Update-ModuleVersion
F28: Remove Set-ExecutionPolicy RemoteSigned from Parse-TransportLogs
(modifies machine security policy as a script side effect)
F30: Add bounds check before split('<')[1] in Parse-TransportLogs
(throws index-out-of-range if no '<' delimiter present in log data)
F31: Replace \\Continue\ global mutation with try/catch for DNS lookup
in Parse-TransportLogs (scoped error handling instead of global state change)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
F03: The final line re-fetched \ via Get-CimInstance but the value was never used. The variable is already assigned earlier in the script where it is consumed by Invoke-Expression slmgr.vbs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Pull request overview
This PR performs a broad PowerShell code-quality audit across the repository, addressing a set of correctness and reliability issues (with security finding F01 intentionally deferred).
Changes:
- Fixes multiple PowerShell runtime issues (e.g., invalid cmdlet usage/typos, incorrect filters, invalid control flow like
break/continueoutside loops). - Reduces operational side effects (e.g., removes interactive remoting patterns; removes execution policy mutation; adds safer parsing bounds checks).
- Improves parameter validation/default correctness and message clarity in several scripts.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Windows/Push-DNSClientServerAddresses.ps1 | Adds mandatory parameters and replaces interactive remoting with Invoke-Command + session cleanup. |
| Windows/Activate and Get License.ps1 | Narrows registry ACL permissions and removes dead code. |
| General/Update-ModuleVersion.ps1 | Makes input version mandatory; fixes prerelease validation message content. |
| General/Purge-InstalledModules.ps1 | Fixes array syntax (missing comma). |
| Exchange/Parse-TransportLogs.ps1 | Removes execution policy side effect; hardens parsing; replaces global error preference mutation with try/catch. |
| DDI/Update-DnsServerList.ps1 | Fixes CIM filter typo so adapter lookup uses the correct index. |
| DDI/Get Hostnames from CSV IP Addresses.ps1 | Improves error handling in hostname resolution loop. |
| Active Directory/Get-ADUserTransitiveGroupMembership.ps1 | Adjusts preference variable setting and fixes pipeline emission behavior. |
| Active Directory/Get-ADObjectFromPipeline.ps1 | Moves pipeline identity resolution into process where pipeline input is available. |
| Active Directory/Get-ADAttributeUniqueValues.ps1 | Fixes default value to match the ValidateSet. |
| Active Directory/Export-AllGroupMemberships.ps1 | Fixes AD filter so $true expands correctly. |
| Active Directory/Export-AllADUserTransitiveGroupMemberships.ps1 | Fixes undefined count reference and updates preference variable setting. |
| Active Directory/Domain Services/Get-FSMORoleDetails.ps1 | Replaces unsupported multi-property -ExpandProperty usage with direct property access. |
| Active Directory/AD Users/Get-LockedOutLocation.ps1 | Adds required AD property and fixes invalid continue usage outside loops. |
| Active Directory/AD Users/Get-InactiveUsers.ps1 | Fixes invalid break usage outside loops. |
| Active Directory/AD Users/Get-InactiveADUser.ps1 | Fixes operator precedence bug; uses ExportCSV value as the export path when provided. |
| Active Directory/AD Groups/Archive-ObsoleteGroups.ps1 | Makes $Domain mandatory, fixes date formatting, and corrects a malformed comment terminator. |
Comments suppressed due to low confidence (3)
Active Directory/Get-ADObjectFromPipeline.ps1:59
- This function is declared as
ValueFromPipeline, but it only emits$User/$Computerin theendblock. With multiple pipeline inputs, this will return only the last processed object (and$User/$Computercan also leak across iterations). Emit results from within theprocessblock (or collect all results in a list and output the list at end) so each pipeline input produces an output.
process {
# Resolve identity type in process block where pipeline input ($Identity) is available.
if ($Identity -is [Microsoft.ActiveDirectory.Management.ADUser]) {
# We have an ADUser object
# Might want to normalize the type to an ADObject IF we can get sidHistory from an ADObject
}
if ($Identity -is [Microsoft.ActiveDirectory.Management.ADComputer]) {
# We have an ADComputer object
# Might want to normalize the type to an ADObject IF we can get sidHistory from an ADObject
}
if ($Identity -is [string]) {
# Find an AD object and determine its type
$Identity = Get-ADObject -Filter "Name -eq `"$Identity`""
}
$IdentityType = $Identity.ObjectClass
switch ($IdentityType) {
'user' {
# Not Complete
$User = Get-ADUser -Identity $Identity -Properties PrimaryGroup,SidHistory
}
'computer' {
# Not Complete
$Computer = Get-ADComputer -Identity $Identity -Properties PrimaryGroup,SidHistory
}
Default {
Write-Error "Identity type not supported."
}
}
}
end {
# Do something and/or return the resulting object to the pipeline.
if ($User) {
$User
}
if ($Computer) {
$Computer
}
}
Active Directory/Get-ADUserTransitiveGroupMembership.ps1:68
ProgressPreferenceis set at global scope toSilentlyContinue, but it is only restored after the connection checks. If the function throws while checking connectivity, the global preference stays modified for the caller’s session. Wrap the temporary preference change in atry/finally(or avoid-Scope Global) to guarantee restoration on all exit paths.
$CurrentProgressPreference = Get-Variable -Name ProgressPreference -ValueOnly
Set-Variable -Name ProgressPreference -Value 'SilentlyContinue' -Force -Scope Global -ErrorAction SilentlyContinue
# Check if the global catalog server is available on the specified port.
if (-not (Test-NetConnection -ComputerName $Server -Port $Port -InformationLevel Quiet -ErrorAction SilentlyContinue)) {
if (-not (Test-NetConnection -ComputerName $Server -Port $AltPort -InformationLevel Quiet -ErrorAction SilentlyContinue)) {
throw "Unable to connect to the global catalog server '$Server' on port '$Port' or '$AltPort.'"
}
}
Set-Variable -Name ProgressPreference -Value $CurrentProgressPreference -Force -Scope Global -ErrorAction SilentlyContinue
}
Active Directory/Export-AllADUserTransitiveGroupMemberships.ps1:158
- This function temporarily sets
ProgressPreferenceat global scope, but it’s only restored after the port checks. If it throws while validating connectivity, the caller’s session can be left withProgressPreference = SilentlyContinue. Usetry/finallyaround the global preference mutation (or avoid global scope) so it’s always restored.
$CurrentProgressPreference = Get-Variable -Name ProgressPreference -ValueOnly
Set-Variable -Name ProgressPreference -Value 'SilentlyContinue' -Scope Global -Force -ErrorAction SilentlyContinue
# Check if the global catalog server is available on the specified port.
if (-not (Test-NetConnection -ComputerName $Server -Port $Port -InformationLevel Quiet -ErrorAction SilentlyContinue)) {
if (-not (Test-NetConnection -ComputerName $Server -Port $AltPort -InformationLevel Quiet -ErrorAction SilentlyContinue)) {
throw "Unable to connect to the global catalog server '$Server' on port '$Port' or '$AltPort.'"
}
}
Set-Variable -Name ProgressPreference -Value $CurrentProgressPreference -Scope Global -Force -ErrorAction SilentlyContinue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full code-quality audit of the repository focused on invalid code, missed logic, and dead code. 31 findings were identified and resolved across 4 severity levels. Security findings (F01) were intentionally excluded from this pass.
Changes by Severity
🔴 Critical (9 fixes)
General/Purge-InstalledModules.ps1Active Directory/Get-ADUserTransitiveGroupMembership.ps1-ValuetoSet-VariableActive Directory/Export-AllADUserTransitiveGroupMemberships.ps1-ValuetoSet-VariableActive Directory/Export-AllGroupMemberships.ps1$trueexpansionActive Directory/Get-ADAttributeUniqueValues.ps1ValidateSetActive Directory/AD Groups/Archive-ObsoleteGroups.ps1$Domainmandatory; fixed 12-hour/single-digit date format; fixed comment block syntaxActive Directory/Domain Services/Get-FSMORoleDetails.ps1-ExpandProperty(unsupported) with direct property accessDDI/Update-DnsServerList.ps1$._Indextypo →$($netadapter.Index)Windows/Push-DNSClientServerAddresses.ps1Enter-PSSession/Exit-PSSessionwithInvoke-Command -Session+Remove-PSSessionin finally block🟠 High (9 fixes)
Active Directory/Get-ADUserTransitiveGroupMembership.ps1endtoprocessblockActive Directory/Export-AllADUserTransitiveGroupMemberships.ps1$UserCount→$Users.CountActive Directory/AD Users/Get-InactiveADUser.ps1-notand comparisonActive Directory/AD Users/Get-InactiveADUser.ps1$ExportCSVvalue now used as export path when providedActive Directory/AD Users/Get-LockedOutLocation.ps1continue→returnin catch block outside any loopActive Directory/Get-ADObjectFromPipeline.ps1begintoprocessblockGeneral/Update-ModuleVersion.ps1$InputVersionmandatory to prevent null member accessWindows/Push-DNSClientServerAddresses.ps1param()blockExchange/Parse-TransportLogs.ps1.DOMAINNAME.orgplaceholder from ConnectionUri🟡 Medium (9 fixes)
Windows/Activate and Get License.ps1FullControl→ReadKeyinRegistryAccessRule(comment said "read permissions")DDI/Get Hostnames from CSV IP Addresses.ps1$error[0]→$_in catch blockActive Directory/AD Groups/Archive-ObsoleteGroups.ps1Active Directory/AD Users/Get-InactiveUsers.ps1break 1→returnin two catch blocks outside any loopActive Directory/AD Users/Get-LockedOutLocation.ps1BadPasswordTimetoGet-ADUser -PropertieslistGeneral/Update-ModuleVersion.ps1$PrereleaseVersion→"$NewVersion$PrereleaseTag"Exchange/Parse-TransportLogs.ps1Set-ExecutionPolicy RemoteSignedside effectExchange/Parse-TransportLogs.ps1split('<')[1]Exchange/Parse-TransportLogs.ps1$ErrorActionPreferenceglobal mutation withtry/catch🔵 Low (1 fix)
Windows/Activate and Get License.ps1$ProductKeyat EOFNot Addressed